fix(kernel): preserve empty metadata filters - #933
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Clean, well-scoped change that stops collapsing empty/blank metadata filters to None and preserves them as real (match-nothing) patterns, with docstrings/CHANGELOG/tests updated to match. One medium concern: the correctness of the new "empty string matches nothing" behavior rests on kernel semantics that the removed comment described as the opposite (kernel rejecting "" with InvalidArgument), and the only tests exercising it are live-warehouse e2e tests — worth confirming against the real kernel and checking whether the ^0.2.0 pin needs bumping.
79b87d7 to
74d883f
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Focused, well-tested fix that flips empty metadata filters from match-all to match-nothing. One medium concern: get_tables passes an empty catalog straight to the kernel while get_schemas/get_columns adapt it via _exact_catalog_and_pattern, and no test verifies tables(catalog_name="") actually matches nothing — worth confirming the kernel's list_tables doesn't treat blank as "all catalogs."
GitHub Actions
Failure
There was a problem hiding this comment.
Verdict: 1 High · 1 Low
One real concern: get_tables is the odd one out — it passes catalog=catalog_name unbridged while get_schemas/get_columns route empty catalogs through the new _exact_catalog_and_pattern helper, so tables(catalog_name="") likely matches all catalogs (or raises) instead of "matches nothing" as the PR's own docstring promises (F1, high). Test coverage for the empty-catalog tables case is also missing (F2, low). The schema/table/column pattern preservation and the empty-catalog bridge for schemas/columns look correct.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-tested behavior change that stops collapsing empty/blank metadata filters to None and forwards them to the kernel unchanged. Unit + e2e coverage is thorough and no references to the removed _none_if_blank/_catalog_or_none helpers remain. One low-severity docstring-consistency nit around how catalog_name is described across the three metadata methods.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Focused, well-tested change that stops collapsing empty/whitespace metadata filters to None on the kernel path. Two concerns: (1) the "empty pattern matches nothing" contract now depends on kernel behavior the connector no longer controls — the removed helper documented that the kernel rejects "" with ProgrammingError, and mocked unit tests can't detect a mismatch, so please confirm/pin the kernel version; (2) the shared abstract base-class docstring now documents kernel-only %/* catalog semantics that don't hold for the Thrift backend.
| stream = self._kernel_session.metadata().list_schemas( | ||
| catalog=_catalog_or_none(catalog_name), | ||
| schema_pattern=_none_if_blank(schema_name), | ||
| schema_pattern=schema_name, |
There was a problem hiding this comment.
🟡 Medium — The change now forwards empty-string pattern filters (schema_pattern, table_pattern, column_pattern) to the kernel verbatim instead of collapsing them to None. The PR description, the base/Cursor docstrings, and the e2e tests all assert that an empty pattern matches nothing (fetchall() == []).
But the previous helper's own docstring documented the opposite kernel behavior: Identifier/LikePattern reject "" with InvalidArgument, which the connector maps to ProgrammingError. If the pinned kernel (databricks-sql-kernel = "^0.2.0", pyproject.toml:62) still rejects "", then empty-string filters now raise ProgrammingError at runtime rather than matching nothing — a regression relative to the documented/asserted contract.
The unit tests (test_get_schemas_preserves_empty_pattern, etc.) mock the kernel session, so they only verify the connector passes "" through — they cannot detect that the real kernel rejects it. Only the real-wheel e2e tests would, and those run in a separate CI step. Please confirm this depends on a coordinated kernel change that accepts "" as match-nothing, and bump the minimum databricks-sql-kernel version (^0.2.0) accordingly so a user on an older kernel doesn't silently get ProgrammingError where the docs promise an empty result set. Applies equally to the call sites at lines 945 and 975-977.
(Anchored to the nearest changed line — see the description for the exact location.)
| catalog_name: Optional catalog name pattern to filter by | ||
| schema_name: Optional schema name pattern to filter by | ||
| catalog_name: Optional exact catalog name. ``None`` leaves the | ||
| filter unset; ``%`` and ``*`` select all catalogs; an empty |
There was a problem hiding this comment.
🔵 Low — This docstring lives on the abstract DatabricksClient base class, which is the shared contract for both the Thrift and kernel backends. It now states catalog_name is an "Optional exact catalog name" where "% and * select all catalogs." That semantics is kernel-only: the Thrift backend passes catalogName=catalog_name straight through (thrift_backend.py:1160/1206/1254) with no %/* normalization, so on Thrift % is a literal catalog name — as the removed _catalog_or_none comment itself noted ("This intentionally diverges from raw-Thrift literalness (Thrift treats % as a literal catalog name)").
A reader of the base contract (and Thrift users) will be misled into thinking catalog_name='%' matches all catalogs on every backend. Consider scoping the wildcard note to the kernel backend, or clarifying that it is a kernel-specific normalization. The same wording appears at databricks_client.py:332 (get_columns) and in the public Cursor docstrings at client.py:1583-1584 and 1641-1642, which are likewise backend-agnostic and user-facing.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
95ec810 to
0f8a432
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Straightforward, well-tested behavior change: empty/whitespace metadata filters are no longer collapsed to None, patterns pass through verbatim, and catalog %/* normalization is retained. Unit + e2e coverage is solid. One medium doc-contract inconsistency: get_tables/tables() describe catalog_name as a "pattern" whose empty value "matches nothing," while get_schemas/get_columns (identical handling) describe it as exact-or-all with empty preserved.
| cursor: The cursor object that will handle the results | ||
| catalog_name: Optional catalog name pattern to filter by | ||
| if catalog_name is None, we fetch across all catalogs | ||
| if catalog_name is None, we fetch across all catalogs; an empty |
There was a problem hiding this comment.
🟡 Medium — The catalog_name docstring for get_tables is inconsistent with the same argument on get_schemas and get_columns, even though all three route catalog_name through the identical _catalog_or_none helper (client.py:924/951/981).
get_schemas(L257) andget_columns(L337): "Optional exact catalog name.Noneleaves the filter unset;%and*select all catalogs; an empty string is preserved."get_tables(L295): "Optional catalog name pattern to filter by / if catalog_name is None, we fetch across all catalogs; an empty string matches nothing."
Since catalog handling is the same in all three (_catalog_or_none maps only None/%/* → unset and passes an empty string through verbatim), the get_tables wording is wrong on two points: it calls the catalog a "pattern" (it's exact-or-all) and claims an empty catalog "matches nothing" (it's actually preserved/passed through, matching the other two). The same divergence exists in the public docstrings in client.py: tables() says "Names are patterns... empty patterns match nothing" (implying catalog too), while schemas()/columns() say catalog is exact with %/* selecting all. Recommend aligning the get_tables/tables() catalog description with get_schemas/get_columns so callers aren't misled about catalog-filter semantics.
(Anchored to the nearest changed line — see the description for the exact location.)
vuanhphung
added
the
kernel-e2e
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
github-actions
Bot
removed
the
kernel-e2e
There was a problem hiding this comment.
Verdict: 1 Medium
Focused, well-tested fix — removing _none_if_blank so empty/whitespace metadata filters reach the kernel unchanged is clean, and unit + e2e coverage matches the new semantics. One medium concern: the public Cursor docstrings (shared by the Thrift and kernel backends) now assert kernel-specific %/* and empty-string semantics unconditionally, which the Thrift path doesn't implement.
|
|
||
| Names can contain % wildcards. | ||
| Empty strings are preserved; ``None`` leaves a filter unset. | ||
| ``catalog_name`` is exact except that % and * select all catalogs. |
There was a problem hiding this comment.
🟡 Medium — These docstrings live on the backend-agnostic Cursor methods, which dispatch to both the Thrift and kernel backends (self.backend.get_schemas/get_tables/get_columns). The new wording — "catalog_name is exact except that % and * select all catalogs" and "empty patterns match nothing" — describes the kernel adapter's normalization (_catalog_or_none + raw pass-through), which is only wired into KernelDatabricksClient. The Thrift backend (thrift_backend.py:1160-1255) forwards catalog_name/schema_name verbatim to catalogName/schemaName with no %/*→all-catalogs collapsing and no empty-string handling; the code comment removed in this PR explicitly noted "Thrift treats % as a literal catalog name." So on the default Thrift path a caller reading these docs will get different behavior than documented. Consider scoping the new semantics to the kernel backend (e.g. "on use_kernel=True, …") or verifying they hold on Thrift before stating them unconditionally.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, intentional behavior change that stops collapsing empty/whitespace metadata pattern filters to None, with matching unit + e2e coverage and an updated CHANGELOG. One low-severity note about an untested empty-catalog edge on get_columns.
| schema_pattern=_none_if_blank(schema_name), | ||
| table_pattern=_none_if_blank(table_name), | ||
| column_pattern=_none_if_blank(column_name), | ||
| schema_pattern=schema_name, |
There was a problem hiding this comment.
🔵 Low — _catalog_or_none no longer collapses blank/whitespace catalogs to None, so columns(catalog_name="") now forwards "" to list_columns. Unlike SHOW SCHEMAS/TABLES (which treat blank as a wildcard), SHOW COLUMNS treats the catalog as an exact Identifier, which the kernel rejects for "" with InvalidArgument → ProgrammingError. The new unit test test_get_columns_preserves_empty_catalog mocks the kernel so it only asserts pass-through and can't surface this; there's no e2e test for an empty catalog on columns() (the e2e empty-filter tests only cover schema/table/column patterns). This is consistent with the PR's stated intent ("exact filters retain the kernel's validation behavior"), but it's a real, previously-working-differently behavior change (columns(catalog="") used to match-all) that goes unverified against the live kernel. Consider an e2e test asserting the actual raised/observed behavior, or a note in the CHANGELOG that empty catalog on columns() now raises rather than matching all.
(Anchored to the nearest changed line — see the description for the exact location.)
1 participant
Fixes PECOBLR-4221.
The kernel metadata adapter no longer collapses empty or whitespace-only filters to
None. Empty pattern filters therefore match nothing, while exact filters retain the kernel's validation behavior. Existing%/*catalog wildcard normalization is unchanged.Testing: 265 focused unit tests passed;
git diff --checkpassed.